Skip to content

make cfg_select a builtin macro #143461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Jul 4, 2025

tracking issue: #115585

This parses mostly the same as the macro cfg_select version, except:

  1. wrapping in double brackets is no longer supported (or needed): cfg_select {{ /* ... */ }} is now rejected.
  2. in an expression context, the rhs is no longer wrapped in a block, so that this now works:
fn main() {
    println!(cfg_select! {
        unix => { "foo" }
        _ => { "bar" }
    });
}
  1. a single wildcard rule is now supported: cfg_select { _ => 1 } now works

I've also added an error if none of the rules evaluate to true, and warnings for any arms that follow the _ wildcard rule.

cc @traviscross if I'm missing any feature that should/should not be included
r? @petrochenkov for the macro logic details

@rustbot rustbot added A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 4, 2025
@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch 2 times, most recently from e3aeb08 to 1dc0034 Compare July 5, 2025 06:03
Comment on lines +186 to +205
#[instrument(skip(cx, tts))]
pub fn expand_token_stream<'cx>(
cx: &'cx mut ExtCtxt<'_>,
sp: Span,
arm_span: Span,
node_id: NodeId,
name: Ident,
tts: TokenStream,
) -> Box<dyn MacResult + 'cx> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to know if there is already a standard way to expand a token stream. I couldn't find one, so I did some refactoring here.

@folkertdev folkertdev marked this pull request as ready for review July 5, 2025 06:07
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR modifies tests/auxiliary/minicore.rs.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@traviscross traviscross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior, per the tests and the impl, looks correct to me.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch 2 times, most recently from 3d68353 to 3abcece Compare July 5, 2025 12:52
@folkertdev
Copy link
Contributor Author

I moved some things around (analogously to asm!) so that rustfmt can use the parsed representation of cfg_select!. Now that it is a builtin macro, I'm assuming formatting won't work and becomes a blocker for stabilization. I think the formatting rules are reasonably straightforward though.

Implementation-wise the one thing I'm not sure about is how to get rustfmt to format a TokenStream that represents the rhs of an arm. It gets expanded based on the context (can be an item, expr, stmt, etc.). But hopefully the rustfmt folks know how to deal with that.

@traviscross
Copy link
Contributor

cc @rust-lang/rustfmt @rust-lang/style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the relevant file for rustfmt: the parse_cfg_select function returns a CfgSelectBranches that keeps (as far as I can tell) all of the structure that is required to format cfg_select.

A cfg_select looks like this

fn print() {
    println!(cfg_select! {
        unix => { "unix" }
        _ => { "not unix" }
    });
}

where the right-hand side of the arrow is a TokenTree. That token tree is expanded based on the context: if the macro is in expression position, it'll be parsed as an expression, similarly for statements, items and any other position where a macro can occur.

So the formatter heeds to handle this TokenTree (really a TokenStream after we strip of the outer braces) somehow. I didn't see immediately how to do that given the existing APIs, but I assume it's possible because macros-by-example would need the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[derive(Default)]
pub struct CfgSelectBranches {
    pub reachable: Vec<(MetaItemInner, TokenStream, Span)>,
    pub wildcard: Option<(Token, TokenStream, Span)>,
    pub unreachable: Vec<(CfgSelectRule, TokenStream, Span)>,
}

Question, what's the difference between reachable, wildcard, and unreachable?

Also, will the TokenTree on the right-hand side of the arrow always be valid rust?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, what's the difference between reachable, wildcard, and unreachable?

For formatting there is really no difference. The goal of this structure is to be able to emit warnings for unreachable branches.

The cfg rules on the left-hand side are evaluated from top to bottom, and the first one that evaluates to true is picked. The wildcard _ always evaluates to true, so any branches that follow it are unreachable.

Also, will the TokenTree on the right-hand side of the arrow always be valid rust?

I don't think so. It is like the right-hand side of a macro_rules! macro rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide examples of more complicated RHS that aren't valid rust?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this

error: macro expansion ignores `+` and any tokens following
  --> /home/folkertdev/rust/rust/tests/ui/macros/cfg_select.rs:30:12
   |
LL | / cfg_select! {
LL | |     _ => { + + + }
   | |            ^
LL | | }
   | |_- caused by the macro expansion here
   |
   = note: the usage of `cfg_select!` is likely invalid in item context

the rhs is a valid token tree, but when expanded it's not valid rust code.

Copy link
Contributor

@ytmimi ytmimi Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt would definitely fail to handle that. Is there a more realistic example you can think of?

In the example above things fail to compile, but is there a case where the RHS is composed of valid tokens that don't parse as valid rust, but still get expanded to valid rust?

Technically, rustfmt doesn't care what the tokens get expanded to as long as the tokens themselves can be parsed as valid rust, otherwise there's no hope to format them. Would you say that the typical case is going to be a RHS that parses as valid rust?

Copy link
Contributor Author

@folkertdev folkertdev Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in valid programs the rhs will generally be valid rust. The only tricky thing is that it's unclear what kind (could be a sequence of items, or an expression, or a statement).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I think it should be enough to call format_snippet. That's how we currently format branches in macro_rules macros.

@bors
Copy link
Collaborator

bors commented Jul 6, 2025

☔ The latest upstream changes (presumably #143521) made this pull request unmergeable. Please resolve the merge conflicts.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 3abcece to 53b3b88 Compare July 6, 2025 15:27
@bors
Copy link
Collaborator

bors commented Jul 7, 2025

☔ The latest upstream changes (presumably #143582) made this pull request unmergeable. Please resolve the merge conflicts.

@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 53b3b88 to 5d550b1 Compare July 7, 2025 18:46
@folkertdev folkertdev force-pushed the cfg-select-builtin-macro branch from 4b84ad7 to 01f41cc Compare July 7, 2025 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants